Skip to content

Fix release process feature freeze info #19148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bukka
Copy link
Member

@bukka bukka commented Jul 16, 2025

@TimWolla
Copy link
Member

TimWolla commented Jul 16, 2025

I don't think this is an accurate change. The policies document says:

Beta 1: Tag / Feature Freeze: $rd + 40 (Aug 13, 2024)

and

After feature freeze, with blessing of the release managers:

  • Merging features that do require an RFC is still allowed.
  • Features that do not require an RFC are still allowed.
  • Optimisations and internal ABI and API changes are also still allowed.

Highlighting mine. This means the policies document defines the feature freeze to happen at the time of Beta 1 and define the feature freeze as “Release Managers need to approve changes that are a feature”, which is a reversal of the process before feature freeze where features may be shipped by gentleman's agreement.

@TimWolla
Copy link
Member

Specifically this also means that RFCs with a non-trivial implementation should be voted on well before the voting deadline, since release managers have the ultimate decision regarding whether or not the implementation is “too risky” to stabilize during Betas and RCs (possibly taking the greater ecosystem into account).

@bukka
Copy link
Member Author

bukka commented Jul 16, 2025

Beta 1: Tag / Feature Freeze: $rd + 40 (Aug 13, 2024)

Ah I missed this one. I think it was kind of left by accident because we specifically put this to the RFC:

Feature freeze is supposed to happen when we start making beta releases, but this does not always happen.

Think it was driven by RFC so the changes to the policy came later IIRC or it was just missed.

But logically if we allow features it shouldn't be called a feature freeze

which is a reversal of the process before feature freeze where features may be shipped by gentleman's agreement.

Previous official process was not to allow any features except the RFC implemenation features. We just did not follow it and merge some features.

Specifically this also means that RFCs with a non-trivial implementation should be voted on well before the voting deadline, since release managers have the ultimate decision regarding whether or not the implementation is “too risky” to stabilize during Betas and RCs (possibly taking the greater ecosystem into account).

Yeah I think that you are right here. It was actaully not an intention but it ended up like this.

Ok I think I need to reword this and probably still keep the name "feature freeze" even though I don't really like it.

@bukka bukka force-pushed the release-process-docs-feature-freeze branch from 596c628 to ccd5b3a Compare July 16, 2025 14:47
@bukka
Copy link
Member Author

bukka commented Jul 16, 2025

Ok I rewrote it to more reflect the policy. So I kind of split it to soft and hard feature freeze. Let me know if that looks better.

4-weeks, 3-weeks, 2-weeks, and 1-week prior to the feature freeze. This is a
recommendation and the intervals may vary based on work load.
the upcoming soft feature freeze by posting reminders to
internals@lists.php.net at 5 weeks, 4 weeks, 3 weeks, 2 weeks, and 1 week prior
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 5 weeks is new, is that intentional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me to have the 5 week reminder, since that gives folks one week to finalize the RFC before needing to start the discussion (= the RFC text will be less rushed).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that was exactly the point to give time for announcing the RFC that can still make it...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even 6 weeks would make more sense then, as that can be announced together with the alpha1 tag?

"We're starting the release procedure now. Here is alpha1. Reminder: You have two weeks to get your RFCs ready for voting."

@TimWolla
Copy link
Member

Let me know if that looks better.

Yes, this looks much better now. I had one minor wording remark, but other than that it LGTM and seems to match both the policy and the “actual practice”.

@bukka bukka force-pushed the release-process-docs-feature-freeze branch from ccd5b3a to d9c1140 Compare July 16, 2025 16:50
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a release manager, but the updates all make sense to me and I don't have any further concerns.

For any RFCs to be included in the new release, they should be discussed and
have their voting polls closed no later than when the first beta is released.
However, this does not mean the new feature must have a complete implementation
by this date. Such implementation can be merged only with RM approval and must
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need RM approval for

  • before hard feature freeze
  • with a successful RFC

those should be done with the normal review process. https://wiki.php.net/rfc/release_cycle_update doesn't make it clear if

The recommendation is to not accept extensive refactoring, but this would not be a hard requirement, and Release Managers must approve any such small feature during the beta period, as is already the case.

if "as is already the case" is about approving small non-RFC features, or all features

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree but the actual policy is quite clear about that: https://github.com/php/policies/blob/06ef24434942f3b09241ccbde124b83ca8a18042/release-process.rst#beta-releases . So we would need to change that which we probably should as I don't like RM being in position to decide so that should change. But for now it's like that and this just reflects the policy...

@edorian
Copy link
Member

edorian commented Jul 17, 2025

My main struggle with this is that neither the policy doc. nor the RFC clearly defines what a "Feature" is. Even the linked Wikipedia article leaves it open to interpretation.

We propose to explicitly allow minor features, optimizations, and internal API / ABI changes, during the beta period.
This is the closest I could find to a definition.

So, to me, the terms "soft" and "hard" feature freeze don't really change the process - they introduce two more labels with specific meanings, but they are defined in the new text. Which is an improvement. Tim mentioned that Debian uses similar terminology, which I found clear and helpful: https://release.debian.org/testing/freeze_policy.html#soft

That said, this mainly needs to work for php-src contributors. So I defer to your judgment - if requiring explicit RM approval for merging voted RFCs between beta1 and RC1 makes sense, I’m on board. But then shouldn't all bigger refactorings require approval?

@Girgias
Copy link
Member

Girgias commented Jul 17, 2025

AFAIK the only time an RM has ever used its right to potentially veto a merge was this comment: #7468 (comment) and that was already in RC.

IMHO RMs should not be able to deny merging of PRs and features as @edorian pointed out this could also be extended to major refactorings, which should be allowed.

Moreover, RMs are not expected to have the php-src knowledge to judge the quality of an implementation. And I don't think I have ever seen a RM block a merge.

@bukka
Copy link
Member Author

bukka commented Jul 19, 2025

My main struggle with this is that neither the policy doc. nor the RFC clearly defines what a "Feature" is

In reality this is usually not a problem as we can usually identify what's feature and what's bug. We should probably define better who can actually decide it if there is a disagreement but I don't really remember situation where we would have a disagreement about that.

That said, this mainly needs to work for php-src contributors. So I defer to your judgment - if requiring explicit RM approval for merging voted RFCs between beta1 and RC1 makes sense, I’m on board. But then shouldn't all bigger refactorings require approval?

This is not supposed to be decided here. This is just a flow to reflect the actual policy in https://github.com/php/policies/blob/06ef24434942f3b09241ccbde124b83ca8a18042/release-process.rst . If anything should change, that's for another discussion.

@bukka
Copy link
Member Author

bukka commented Jul 19, 2025

IMHO RMs should not be able to deny merging of PRs and features as @edorian pointed out this could also be extended to major refactorings, which should be allowed.

Moreover, RMs are not expected to have the php-src knowledge to judge the quality of an implementation. And I don't think I have ever seen a RM block a merge.

I actually agree with that but unfortunately that's not what is in the policy atm 😞

Although we could get some sort of "auto approval" from @edorian and/or @DanielEScherzer and then we wouldn't need to care about asking them. :)

I think ideally going forward it would be better to rather require approval from code owners / maintainers for any such feature as they should be in much better position to decide the impact...

@DanielEScherzer
Copy link
Member

IMHO RMs should not be able to deny merging of PRs and features as @edorian pointed out this could also be extended to major refactorings, which should be allowed.
Moreover, RMs are not expected to have the php-src knowledge to judge the quality of an implementation. And I don't think I have ever seen a RM block a merge.

I actually agree with that but unfortunately that's not what is in the policy atm 😞

Although we could get some sort of "auto approval" from @edorian and/or @DanielEScherzer and then we wouldn't need to care about asking them. :)

If the policy is that release managers should approve things, I'm not sure I'm comfortable with auto approval. But, as it happens I do think I have the php-src knowledge to give at least a cursory review and a "You have my blessing as a release manager". Maybe for the next version of PHP we revisit this policy, but for now leave as-is? If I'm pinged I should be able to review fairly quickly (1-2 days at most)

@TimWolla
Copy link
Member

TimWolla commented Jul 19, 2025

IMHO RMs should not be able to deny merging of PRs and features as @edorian pointed out this could also be extended to major refactorings, which should be allowed.

I disagree. For RFCs an accepted vote just means that the community agreed they want a specific change in the language. It does not mean that the change is guaranteed happen in the next PHP version. The PDO subclasses RFC or one of the PHP 8.4 deprecations come to mind. The RFC template is also explicitly specifying the use of relative target versions: https://wiki.php.net/rfc/template#proposed_php_version_s

According to the current policy RMs need to approve changes to the language after the feature freeze happened. And I believe this is a good thing: It's the job of the RMs to ensure that the version they are responsible for is stable and in a good shape.

If the RMs have reason to believe that there is not sufficient time to stabilize a feature in the 6 weeks until RCs / hard freeze - at this point the internal API / ABI will also become locked in - they should absolutely be able to veto a feature for the given PHP version and defer to the next one. It does not mean that they are capable of vetoing an accepted RFC entirely, it will just get deferred.

I trust the RMs to use this responsibly and in the best interest of the PHP community.

Moreover, RMs are not expected to have the php-src knowledge to judge the quality of an implementation.

Quoting from the RM announcement for PHP 8.5 (https://externals.io/message/126733#126960):

Candidates should have a reasonable knowledge of internals, be confident about merging pull requests without breaking backward compatibility, doing triage for bugs, liaising with previous release managers, and generally getting the branch in good shape, as these are among the activities you will be undertaking as release manager. Ideally, at least one of candidate should be a core developer that can assess more technical PR's. Other candidates do not necessarily need to have deep knowledge of internals but should understand above mentioned points.

(Highlighting mine)

I absolutely expect that RMs are capable of judging the implementation complexity of a PR, e.g. by looking at the tests, the size of the PR, the review notes or the amount of review cycles. I also expect them to take the greater ecosystem into account in their judgement. Shipping syntax changes in Beta 3 where it's reasonable that folks have already started updating static analyzers or breaking internal APIs after external extensions have already adjusted their code for compatibility might be too disruptive when there is no way to revert them in the RCs.

But, as it happens I do think I have the php-src knowledge to give at least a cursory review and a "You have my blessing as a release manager".

Given it's only affecting a 6-week period (from soft freeze until RC 1 / hard freeze) I think it would be absolutely reasonable for folks to request RM review when they believe the implementation is done and then for the RM to give a quick ACK that this seems sufficiently safe.


To give a specific example as to when such a veto could have been helpful: Lazy Objects in PHP 8.4 was a last-minute RFC (voting ended 3 days before feature freeze) and the 13000 (!) line implementation PR was merged for the last Beta. If folks only test actual releases and not "nightly builds" this means that any bugs or "design issues" could only be fixed with RC 1 or later when API is locked in. In the end it turned out the RFC missed one reflection method for the feature to be useful in practice and then after careful deliberation - and in violation of the rules - it was decided to treat the missing method as a "bug in the RFC" and include it, despite technically being a feature that was not agreed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants